Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

test: relax chunk count expectations #25415

Merged
merged 1 commit into from
Jan 20, 2019
Merged

test: relax chunk count expectations #25415

merged 1 commit into from
Jan 20, 2019

Conversation

gireeshpunathil
Copy link
Member

In parallel/test-fs-read-stream-concurrent-reads.js the number of data chunks used is being tested when few concurrent reads are performed. The number of chunks can fluctuate based on the
number of concurrent reads as well as the data that was read in one shot. Accommodate these variations in the test.

Rational for M * 2 in the assertion statement:
Number of chunks are clearly a function of the (for) loop iteration, and hence M
Number of chunks are rarely increase based on the number of iterations it took complete one read. Mostly one, rarely 2. Accommodate this fluctuation by taking the worst case situation: all the reads take 2 iterations. And hence M * 2.

Fixes: #22339

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

ping @nodejs/testing @addaleax @Trott

@nodejs-github-bot
Copy link
Collaborator

@gireeshpunathil build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/2204/pipeline

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Jan 9, 2019
@gireeshpunathil
Copy link
Member Author

CI: https://ci.nodejs.org/job/node-test-pull-request/20096/

@gireeshpunathil gireeshpunathil added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 14, 2019
@gireeshpunathil
Copy link
Member Author

Resume Build CI: https://ci.nodejs.org/job/node-test-pull-request/20100/

@addaleax addaleax mentioned this pull request Jan 14, 2019
Closed
@addaleax
Copy link
Member

I do not understand the PR description, tbh … the number of chunks should not be as relevant as the number of buffers – and as we are trying to re-use buffers as much as possible in the ReadStream implementation, I don’t think an 8× memory overhead would be within expected limits?

@gireeshpunathil
Copy link
Member Author

sorry if I was vague. Here is the proof:

index 32a6cd6..9b3af76 100644
--- a/test/parallel/test-fs-read-stream-concurrent-reads.js
+++ b/test/parallel/test-fs-read-stream-concurrent-reads.js
@@ -14,6 +14,7 @@ const filename = fixtures.path('loop.js');  // Some small non-homogeneous file.
 const content = fs.readFileSync(filename);
 
 const N = 1000;
+const M = process.argv[2] / 1;
 let started = 0;
 let done = 0;
 
@@ -34,14 +35,15 @@ function startRead() {
       if (++done === N) {
         const retainedMemory =
           [...arrayBuffers].map((ab) => ab.byteLength).reduce((a, b) => a + b);
-        assert(retainedMemory / (N * content.length) <= 3,
-               `Retaining ${retainedMemory} bytes in ABs for ${N} ` +
-               `chunks of size ${content.length}`);
+        console.log(`growth: ${arrayBuffers.size}, concurrency: ${M}`)
+        // assert(retainedMemory / (N * content.length) <= 3,
+        //        `Retaining ${retainedMemory} bytes in ABs for ${N} ` +
+        //       `chunks of size ${content.length}`);
       }
     }));
 }
 
 // Don’t start the reads all at once – that way we would have to allocate
 // a large amount of memory upfront.
-for (let i = 0; i < 4; ++i)
+for (let i = 0; i < M; ++i)
   startRead();
$  ./node test/parallel/test-fs-read-stream-concurrent-reads.js 4
growth: 5, concurrency: 4
$  ./node test/parallel/test-fs-read-stream-concurrent-reads.js 5
growth: 6, concurrency: 5
$  ./node test/parallel/test-fs-read-stream-concurrent-reads.js 10
growth: 11, concurrency: 10
$  ./node test/parallel/test-fs-read-stream-concurrent-reads.js 100
growth: 101, concurrency: 100
$

so clearly the number of buffers (64K entries in the set) is a function of the concurrent reads, and hence I associated the predicate with that, labelling as M.

M * 2 => is where probably we can argue: For every read, if the actual file read took 2 steps instead of one (for unknown reasons related to stream internal logic, file system logic, load on the system etc.) then 2 buffers are allocated instead of one.

@gireeshpunathil
Copy link
Member Author

ok, I got your response in the main issue; so will have a look and follow your suggestion, thanks.

@addaleax
Copy link
Member

@gireeshpunathil Okay, I think we basically figured out the same things :) So yeah, the issue is that we have too many concurrent reads for too little actual content.

@addaleax
Copy link
Member

(We may want to think about a strategy to fix this in the stream implementation itself, btw – It’s not great that 100 concurrent reads allocate 100 full-sized buffers. I’m not sure how to work around that, though.)

@gireeshpunathil
Copy link
Member Author

@addaleax - pushed a change to that effect; ptal. minor deviation is in terms of N :

N = (pool size) * (concurrent reads) / (file size)
  = 65536 * 6 / 170
  = 2313

so I took 2000 on the safer side.

addaleax
addaleax approved these changes Jan 14, 2019
View reviewed changes
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM :)

@addaleax
Copy link
Member

CI: https://ci.nodejs.org/job/node-test-pull-request/20105/

@gireeshpunathil
Copy link
Member Author

Resume Build CI: https://ci.nodejs.org/job/node-test-pull-request/20150/

This was referenced Jan 16, 2019
Closed
Closed
Closed
@gireeshpunathil
test: relax chunk count expectations
cc26957 
In parallel/test-fs-read-stream-concurrent-reads.js the number
of data chunks used is being tested when few concurrent reads
are performed. The number of chunks can fluctuate based on the
number of concurrent reads as well as the data that was read in
one shot. Accommodate these variations in the test.

Fixes: nodejs#22339

PR-URL: nodejs#25415
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@gireeshpunathil gireeshpunathil removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 20, 2019
@gireeshpunathil gireeshpunathil merged commit cc26957 into nodejs:master Jan 20, 2019
@gireeshpunathil
Copy link
Member Author

landed as cc26957

addaleax pushed a commit that referenced this pull request Jan 23, 2019
@gireeshpunathil @addaleax
test: relax chunk count expectations
faa1776 
In parallel/test-fs-read-stream-concurrent-reads.js the number
of data chunks used is being tested when few concurrent reads
are performed. The number of chunks can fluctuate based on the
number of concurrent reads as well as the data that was read in
one shot. Accommodate these variations in the test.

Fixes: #22339

PR-URL: #25415
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Jan 24, 2019
Merged
BethGriggs pushed a commit that referenced this pull request Apr 29, 2019
@gireeshpunathil @BethGriggs
test: relax chunk count expectations
1557274 
In parallel/test-fs-read-stream-concurrent-reads.js the number
of data chunks used is being tested when few concurrent reads
are performed. The number of chunks can fluctuate based on the
number of concurrent reads as well as the data that was read in
one shot. Accommodate these variations in the test.

Fixes: #22339

PR-URL: #25415
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@BethGriggs BethGriggs mentioned this pull request May 1, 2019
Merged
BethGriggs pushed a commit that referenced this pull request May 10, 2019
@gireeshpunathil @BethGriggs
test: relax chunk count expectations
f5ee585 
In parallel/test-fs-read-stream-concurrent-reads.js the number
of data chunks used is being tested when few concurrent reads
are performed. The number of chunks can fluctuate based on the
number of concurrent reads as well as the data that was read in
one shot. Accommodate these variations in the test.

Fixes: #22339

PR-URL: #25415
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
MylesBorins pushed a commit that referenced this pull request May 16, 2019
@gireeshpunathil @MylesBorins
test: relax chunk count expectations
b4b4c11 
In parallel/test-fs-read-stream-concurrent-reads.js the number
of data chunks used is being tested when few concurrent reads
are performed. The number of chunks can fluctuate based on the
number of concurrent reads as well as the data that was read in
one shot. Accommodate these variations in the test.

Fixes: #22339

PR-URL: #25415
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
This was referenced May 29, 2019
Closed
Closed
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lpinca lpinca lpinca approved these changes

@addaleax addaleax addaleax approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Investigate flaky parallel/test-fs-read-stream-concurrent-reads
5 participants
@gireeshpunathil @nodejs-github-bot @addaleax @jasnell @lpinca